-
Notifications
You must be signed in to change notification settings - Fork 551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalize certificate flag names #1868
Conversation
cc @znewman01 |
This changes the flag names to use certificate instead of the abbreviated cert. To avoid breaking clients, we add a global alias that translates between the two. Signed-off-by: Hayden Blauzvern <[email protected]>
276e113
to
e2b62ca
Compare
Codecov Report
@@ Coverage Diff @@
## main #1868 +/- ##
==========================================
- Coverage 33.44% 33.37% -0.08%
==========================================
Files 146 146
Lines 9340 9360 +20
==========================================
Hits 3124 3124
- Misses 5843 5863 +20
Partials 373 373
Continue to review full report at Codecov.
|
Signed-off-by: Hayden Blauzvern <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to do it as a normalization function or as an alias? I think viper supports RegisterAlias
which we use elsewhere in this repo, which to me feels like a better match for what we're actually doing here.
@@ -41,8 +41,8 @@ cosign attest [flags] | |||
``` | |||
--allow-insecure-registry whether to allow insecure connections to registries. Don't use this for anything but testing | |||
--attachment-tag-prefix [AttachmentTagPrefix]sha256-[TargetImageDigest].[AttachmentName] optional custom prefix to use for attached image tags. Attachment images are tagged as: [AttachmentTagPrefix]sha256-[TargetImageDigest].[AttachmentName] | |||
--cert string path to the X.509 certificate in PEM format to include in the OCI Signature | |||
--cert-chain string path to a list of CA X.509 certificates in PEM format which will be needed when building the certificate chain for the signing certificate. Must start with the parent intermediate CA certificate of the signing certificate and end with the root certificate. Included in the OCI Signature | |||
--certificate string path to the X.509 certificate in PEM format to include in the OCI Signature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the help text reflect that these flags are aliased?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using RegisterAlias
for cosign sign-blob
to alias output
and output-signature
. One downside is the help text shows each of these flags on separate lines, so it looks like they're two separate flags.
Besides that, aliasing and normalizing seem pretty similar. I prefer normalize because I ideally don't want to continue to support the old flag names - Maybe we can drop the normalize function in a 2.0 release?
Another benefit of normalize over aliasing is I don't have to alias the flag in each command, there's no GlobalRegisterAlias like with normalize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One downside is the help text shows each of these flags on separate lines, so it looks like they're two separate flags.
Ugh 😭
I prefer normalize because I ideally don't want to continue to support the old flag names
That's a sufficient argument for me.
This changes the flag names to use certificate instead of
the abbreviated cert. To avoid breaking clients, we add
a global alias that translates between the two.
Signed-off-by: Hayden Blauzvern [email protected]
Summary
Ticket Link
Fixes #1847
Release Note